-
Notifications
You must be signed in to change notification settings - Fork 913
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make isinstance check pass for proxy ndarrays #16601
Make isinstance check pass for proxy ndarrays #16601
Conversation
… into feat/ndarray-instance-check
… into feat/ndarray-instance-check
/ok to test |
This PR fixes the two issues with #16286 (ie. the previous version of this PR that we reverted). The two issues were:
The fix for 2. leads to a problem which the only way I can think of solving is via monkeypatching. The problem is that now that our proxy array passes the check Take if (PyArray_Check(obj)) {
void* buffer = PyArray_DATA(obj) // access the buffer directly
} I think the only way to avoid this problem from Python is to monkeypatch every function like There is at least one other function in a third-party library which we'll need to patch ( Numba CPU dispatched functions also access the proxy arrays buffer. The good news is that eventually numba will support compiling objects which implement What do you all think of the monkey patching approach? Another idea would be pay the cost of the DtoH transfer upfront (like in the previous PR). The DtoH is only done on instance creation. And then after that, the fast-slow proxy mechanism is used. The pro here is that the buffer will be set correctly, and thus no monkeypatching. The con is obviously the DtoH transfer. |
/ok to test |
Summary of offline discussion: monkey-patching numpy to make this work is a bridge too far without much stronger motivations, and we will probably just go with the eager D2H copy instead. |
…rray-instance-check
Ping @vyasr for a review next week |
@Matt711 The PR description says "do not merge" but there is no "DO NOT MERGE" label. Can you make this consistent? Also for team knowledge, the "Description" section of the PR body is used in the final commit message when the PR is merged. Temporary information like the PR state or benchmarks are better to put in comments rather than the "Description" section. |
…rray-instance-check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of suggestions for improvement, but assuming you don't object to applying them I don't need to review again.
/ok to test |
/okay to test |
/okay to test |
/merge |
Closes rapidsai#14537. Authors: - Matthew Murray (https://github.com/Matt711) - GALI PREM SAGAR (https://github.com/galipremsagar) Approvers: - GALI PREM SAGAR (https://github.com/galipremsagar) - Matthew Roeschke (https://github.com/mroeschke) - Vyas Ramasubramani (https://github.com/vyasr) URL: rapidsai#16601
The torch test should no longer fail after #16601. Authors: - Matthew Murray (https://github.com/Matt711) Approvers: - James Lamb (https://github.com/jameslamb) - Matthew Roeschke (https://github.com/mroeschke) URL: #16705
Proxy numpy arrays now instances of real numpy arrays (#16601), so libraries (eg. numba, torch) which utilize NumPy's C API should now be able to use proxy arrays. This PR updates the cudf.pandas documentation to reflect this. Authors: - Matthew Murray (https://github.com/Matt711) Approvers: - Matthew Roeschke (https://github.com/mroeschke) URL: #16697
Closes rapidsai#14537. Authors: - Matthew Murray (https://github.com/Matt711) - GALI PREM SAGAR (https://github.com/galipremsagar) Approvers: - GALI PREM SAGAR (https://github.com/galipremsagar) - Matthew Roeschke (https://github.com/mroeschke) - Vyas Ramasubramani (https://github.com/vyasr) URL: rapidsai#16601
Description
Closes #14537.
Checklist